db/state: don't clear domain RAM under a published SharedDomains - #23046
Conversation
Read views over a published SD keep DomainReader pointing at the SD's in-memory domain maps. The background-commit teardown (bgSD.Close -> mem.Close -> ClearRam) emptied those maps while RPC readers still held them: an in-flight receipt read missed silently (ok=false, err=nil), fell back to the request's pre-commit tx, and ReceiptAsOf zero-filled the miss - GetReceiptsGasUsed returned GasUsed=0 for every tx of the head block and eth_feeHistory reward percentiles were silently wrong. Events.PublishOverlay now marks the SD's TemporalMemBatch as published, and ClearRam on a published batch is a no-op: Close still releases writer resources, while the maps go to the GC once the last reader drops the pointer. The flag lives on the batch itself so every clear path respects it, not only SharedDomains.Close. TestEmbeddedRPCCacheViewDoesNotRefillCodeOfDeletedAccount pinned the old fallback (a post-teardown view re-reading pre-deletion state from its own tx); the view now keeps serving the published head, so the deletion stays visible. The cache non-refill invariant is unchanged.
AskAlexSharov
left a comment
AskAlexSharov
left a comment
There was a problem hiding this comment.
mutable atomic field on shared object? (which used by exec
| // published is set once readers may hold this batch's in-memory maps (an SD | ||
| // published to RPC readers). ClearRam then becomes a no-op: the maps go to | ||
| // GC with the last reader instead of being emptied under it. | ||
| published atomic.Bool |
There was a problem hiding this comment.
Atomic mutable field on shared object - usually means "it's a hack".
it looks like Reader just forever disabling ClearRam() - so it's sounds like memory leak.
"SD published" - does Exec "publishing many SD objects"? Where is it in code?
In my head: RPC starting "ReadView" short-living object which holding consistency of data for RPC method time. So, i think it's job of "ReadView begin/end" methods to manage "what memory needs to hold, and what can free".
Or i miss-understand SD object lifetime?
There was a problem hiding this comment.
-
At construction the batch can't know it will be shared with readers — that only becomes true later, at Events.PublishOverlay. So it's a flag set once, never cleared. It guards readers: clearing the maps under them would make reads silently return zeros
-
The publisher sets the flag (Events.PublishOverlay), not the reader. No leak: one SD per updateForkChoice, at most one published at a time, and the GC frees the maps once the last in-flight RPC view drops the pointer
-
currentContext is a local variable of updateForkChoice (created at forkchoice.go:394): each forkchoice creates its own, publishes it once (:835, the only publication point), withdraws it (PublishOverlay(nil)) and closes it before returning — or hands it to the bg-commit goroutine as bgSD, which does the same
-
The views have no end-of-life hook today — they are created in rpchelper.WithOverlay/the receipts generator and dropped, not closed, so begin/end means refcounting on every creation site. I followed the fix @yperbasis suggested in the rpc: resolve eth_feeHistory on the block overlay view #22987 review:
" don't clear domain RAM under a published SD — [...] leaving the maps to GC once Events and the last view drop the pointer. Refcounting is the heavier alternative".
The flag only selects that teardown variant at the single publication point. @yperbasis please correct me if I've misread your suggestion — happy to go a different way if you two prefer a different approach.
There was a problem hiding this comment.
- i understand what flag doing. point is: if object has 2 different lifetimes (
write+clearRam+continue_writes_to_same_objectandwrite+publish+assume_that_no_future_writes_will_happen): then it's race from all directions: "writer can call ClearRam 1millisecond before reader set atomic field", "writer will continue write to same object without clearing ram - which will be unexpected for Writer and for Reader", etc... Only real solution here is: choose 1 lifetime of SD object. If it's "write+publish" then remove "ClearRam" method, if it's "w
rite+clear_ram+write_more_to_same_object" then remove "Publish" method.
also adding "never clear ram" method to object which was used in "clear ram" mechanic (and living long time) - sounds like source of mem-leaks
- If Publisher set the flag and Publisher calling ClearRam - then why "Publisher protecting from himself"? Publisher already knows - when he wants Publish and when he wants ClearRam - unclear why need add field inside object about it. Feels like biz-logic leaking from higher-level to lower-level code.
Review feedback: an object serving two lifetimes (clear-and-reuse vs publish-and-drop) coordinated by an atomic flag keeps the ambiguity alive. Pick one lifetime instead: Close only releases writer resources and always leaves the in-memory maps to the GC - an unpublished batch is dropped right after Close anyway, and a published one may still have readers. ClearRam stays as the explicit operation of owners that clear and reuse a batch, which never publish it.
|
@AskAlexSharov . You're right — I picked one lifetime. Close now never clears the RAM: it only releases writer resources, and the maps always go to the GC. |
After Close stopped clearing, ClearRam had no production caller left: its only remaining user was an internal test mimicking the integration tool's OLD loop (reuse one SharedDomains across batches). The tool itself creates a fresh SharedDomains per batch today, so the test now mirrors that; the invariants it pins (BranchCache coherence via Commit, state-reader restore) are unchanged and stay green in both exec modes. With the method gone the batch has a single lifetime - write, maybe publish, close-and-drop - and no API can clear the maps under readers.
|
Execution from 0 will create new SD object every batch? |
2 participants
Fixes the high-severity finding from the #22987 review.
Problem. RPC read views keep a
DomainReaderpointing at the published SD's in-memory domain maps. The background-commit teardown (bgSD.Close()→mem.Close()→ClearRam()) emptied those maps while readers were still using them. A receipt read of the in-flight block then missed silently, fell back to the request's pre-commit tx, andReceiptAsOfzero-filled the miss:GetReceiptsGasUsedreturnedGasUsed=0for every tx of the head block, andeth_feeHistoryreward percentiles were silently wrong. Latest-state reads could likewise fall back to the previous block's state mid-request.Fix.
TemporalMemBatch.Closeno longer clears the in-memory domain maps: they go to the GC once the last reference drops. With that,ClearRamhad no production caller left and is removed entirely — the batch has a single lifetime (write, maybe publish, close-and-drop) and no API can clear the maps under readers. The one internal test that used clear-and-reuse now mirrors whatcmd/integrationactually does today: a freshSharedDomainsper batch.Tests. New
TestClose_KeepsDomainRamForReaders(red before the fix, green after). One existing assert updated: a post-teardown view now keeps serving the published head instead of falling back to its own tx.#22987 (draft) depends on this PR: pinning the overlay across
Forkmakes this window easier to hit, so that PR stays a draft until this one is merged.